Core: Allow configuring metrics reporter impl via Catalog property#6404
Core: Allow configuring metrics reporter impl via Catalog property#6404rdblue merged 2 commits intoapache:masterfrom
Conversation
| new BaseTable( | ||
| ops, | ||
| fullTableName(loadedIdent), | ||
| report -> reportMetrics(tableIdentifier, report, session::headers)); |
There was a problem hiding this comment.
Just distantly related and only for my own information: With the current implementation MetricsReporter could be configured when we start the Catalog and then all the tables within this catalog would use the same MetricsReporter. I'm wondering if it would make sense to make this setting more flexible, like being able to set this even after the Catalog is up and running, and additionally have the ability to use different MetricsReporter implementation for different tables.
I don't have any specific use-cases in mind, one could be when let's say querying a table becomes pretty slow at some point and we want to switch to another MetricsReporter that could help us analysing the scan metrics to spot the issue, and then switch back to LoggingMetricsReporter once the issue is solved. (Assuming that in the future we gonna have way more scan metrics that could be used for debugging such issues)
This is not meant to be covered in this PR just thinking out loud if it makes sense.
There was a problem hiding this comment.
like being able to set this even after the Catalog is up and running
I'm not sure this is want we want. I would probably say it's better/easier to just re-configure/re-build the catalog with a different metrics reporter. This is what we do across other catalog properties as well.
have the ability to use different MetricsReporter implementation for different tables.
I don't think the UX will be great in such a case where a user would have to re-configure the metrics reporter per table. I think if we'd want to support something like that, then we'd need a strong use case that justifies this.
This is just my personal opinion, but I'm happy to hear what others think on that topic. If there's enough interest/a strong use case then we can re-evaluate.
There was a problem hiding this comment.
I agree with @nastra on these.
I don't think that the metrics reporter needs to be very flexible because it isn't a user setting or a table setting. It is almost certainly something that will be handled by platform administrators. That means injecting it per catalog is the right thing to do.
nastra
force-pushed
the
configurable-metrics-reporter
branch
from
f914a04 to
475d946
Compare
api/src/main/java/org/apache/iceberg/metrics/LoggingMetricsReporter.java
Show resolved
Hide resolved
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java
Outdated
Show resolved
Hide resolved
nastra
force-pushed
the
configurable-metrics-reporter
branch
from
475d946 to
5814c52
Compare
|
Thanks, @nastra! |
3 participants
In certain cases it makes sense to make the used metrics reporter customizable, so that users have more control over it and how it's reporting